Skip to content

Superclass modifying recipes#3

Merged
timtebeek merged 27 commits intoopenrewrite:mainfrom
Fossur:superclass-modifying-recipes
Apr 28, 2025
Merged

Superclass modifying recipes#3
timtebeek merged 27 commits intoopenrewrite:mainfrom
Fossur:superclass-modifying-recipes

Conversation

@Fossur
Copy link
Copy Markdown
Collaborator

@Fossur Fossur commented Mar 31, 2025

This has recipes to update/remove various supertypes basically. A big part of this is a recipe that can add method stubs when changing classes. Basically it also has various cleanup method and visitors to maintain type consistency after removing/updating methods - I did want it to be as generalizable as possible to also aid in other migrations potentially

Anything in particular you'd like reviewers to focus on?

If there are cleaner ways of adding the methods, doing various type checks, then I would really be open to it. This is a big PR - so take your time - it made sense to bundle them because they are all somewhat related.

Anyone you would like to review specifically?

@timtebeek

Any additional context

I also have some general questions regarding the migrations, basically how do we want to add new files like Security Configuration or helper classes - there are a couple of ways we could go about it 1) We can add files via recipes like this - essentially would work, but 1) not good for multi-module projects 2) we do not know where to put those files 3) We lose the type information and these can't really be operated on. The other way is a helper library, where we could put those files, like SecurityConfiguration, a DAO helper class - would you be open to this idea? Any alternative are welcome as well

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Mar 31, 2025
@Fossur Fossur marked this pull request as ready for review April 1, 2025 05:18
@timtebeek timtebeek self-requested a review April 1, 2025 19:31
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Fossur
Copy link
Copy Markdown
Collaborator Author

Fossur commented Apr 7, 2025

👋 Any feedback on this?

@timtebeek
Copy link
Copy Markdown
Member

I'll try to get this in with a first 0.0.1 release tomorrow, but haven't found the time to go over it yet. 🙏🏻

@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Apr 7, 2025
targetSupertypeName: "io.dropwizard.hibernate.AbstractDAO"
- org.openrewrite.java.dropwizard.method.ChangeSuperType:
targetClass: "io.dropwizard.hibernate.AbstractDAO"
newSuperclass: "ee.taltech.example.AbstractJpaDAO" # Custom class; from helper library
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing any reference to this one; might that be internal to what you're developing there?

Copy link
Copy Markdown
Collaborator Author

@Fossur Fossur Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have some general questions regarding the migrations, basically how do we want to add new files like Security Configuration or helper classes - there are a couple of ways we could go about it 1) We can add files via recipes like this - essentially would work, but 1) not good for multi-module projects 2) we do not know where to put those files 3) We lose the type information and these can't really be operated on. The other way is a helper library, where we could put those files, like SecurityConfiguration, a DAO helper class - would you be open to this idea? Any alternative are welcome as well

Essentially I wanted to discuss this, eg. the dropwizard-hibernate library gives a DAO superclass basically - the dropwizard-hibernate .jar itself is riddled with transitive dependencies that bring much of other dw dependencies also in. To get rid of as much of the preexisting dropwizard dependencies themselves I basically isolated the DAO class (with some modifications and put it to a helper library) - I was wondering if that is an approach that is valid here or no

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable that you'd want to trim that down quite a bit, especially in house. For the recipes we offer here I think it might be best not to make any surprising code changes for folks, so I've commented out this step for now so we can get the rest of the changes in.

Comment on lines +65 to +74
spec.parser(JavaParser.fromJavaVersion().classpath("argparse4j", "spring-boot")),
java(
"""
import net.sourceforge.argparse4j.impl.Arguments;
import net.sourceforge.argparse4j.inf.Subparser;
import org.springframework.boot.CommandLineRunner;

public class RenderCommand implements CommandLineRunner {
@Override
public void configure(Subparser subparser) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of an odd mix of classes used here: it seems to test that halfway through the migration we can clear out an override. Would it not make more sens to have a unit test for the full declarative recipe verify that a fully Dropwizard input is converted to the expected Spring Boot equivalent?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first class has an invalid Override method, the second checks that it is removed, I took the example test case from my example dw service repository, it can be repurposed though, what do you suggest?

import static org.openrewrite.java.tree.TypeUtils.asFullyQualified;
import static org.openrewrite.java.tree.TypeUtils.isAssignableTo;

public class AddMissingAbstractMethods extends Recipe {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, and optionally, we already have AddMissingMethodImplementation, which is a little more explicit in it's use, but then saves us having to maintain two recipes for similar functionality. Please let me know your thoughts!

Copy link
Copy Markdown
Collaborator Author

@Fossur Fossur Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I made this recipe before the other one existed, I wanted to make a recipe that can handle all kinds of type changes for different migration scenarios - For more of the overall context; I ran into an issue where entire method signatures change here. I realized that it's really a really a lot of work to start reworking existing methods into new forms, and much simpler to add scaffolding here - so I focused on the quality of life aspect here to automate is as much as I can. I didn't find a really good way of doing so though; I initially kinda hoped that I could just get the method information from the superclasses and kinda reput it into the classes that do the implementation, but that approach did not work, no convenient way to make the Method back into MethodDeclaration form - so I resorted to manually parsing parameters here. Ultimately, its your guys call here how you want to maintain this stuff.

Copy link
Copy Markdown
Collaborator Author

@Fossur Fossur Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would necessarily interfere with the other recipe, as ideally this could be turned on/off at will, but I understand the maintenance concerns - so your call here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the AddAbstractMethods from the PR here, still waiting for feedback on the other stuff

@timtebeek
Copy link
Copy Markdown
Member

Did a first pass through, but more review / polish is needed before we can confidently merge this in. From my point of view some opportunities to reuse existing recipes were missed, which then leads to (not exact) duplication. As long term maintainer of these recipes I think it'd be good to reduce that duplication where we can, as we'd have to otherwise keep up functional copies whenever we make internal changes. Let me know if you'd be open to making those changes still!

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • build.gradle.kts
    • lines 14-13

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek self-requested a review April 20, 2025 08:32
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 110-109

Copy link
Copy Markdown
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the continued work here @Fossur , and the patience with the reviews. I've taken some shortcuts to be able to get this in right now, be stripping out any (up to this point) unnecessary steps such that there's less to review and maintain. I hope you agree!

@timtebeek timtebeek merged commit a54064d into openrewrite:main Apr 28, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Apr 28, 2025
@timtebeek
Copy link
Copy Markdown
Member

Also: let me know when you'd consider the recipes here fit for a v0.0.1, and then we'll push that out and add it to the docs. 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants